Skip to content

Conversation

@erikjanwestendorp
Copy link
Contributor

Description

This PR belongs to: #6466.

  • Change UmbracoApiController to a default Controller
  • Added route attributes
  • Delete obsolete warning

Type of suggestion

  • Typo/grammar fix
  • Updated outdated content
  • New content
  • Updates related to a new version
  • Other

Product & version (if relevant)

Umbraco CMS v14 & 15

Copy link
Contributor

@sofietoft sofietoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @erikjanwestendorp ! 💪
Just a single comment from my side.

@erikjanwestendorp
Copy link
Contributor Author

@sofietoft Thanks for the review, just updated the PR.

@sofietoft
Copy link
Contributor

Thanks @erikjanwestendorp ! 💪

I've been testing this, but cannot get it to work 😅 Seems I need to setup a lot of things around it, in order to test properly.

I'll pass this one on to our developers for a confirmation 💪

@sofietoft sofietoft added review/developer Use this label if an internal developer review is required category/umbraco-cms labels Oct 7, 2024
Copy link
Contributor

@sofietoft sofietoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had this past one of our developers, and it looks like the code in the test is using some legacy methods from the NUnit framework.
So, a few changes should be made here:

Line 200: Swap AreEqual with That

Assert.That(expected == result);

Line 206: Change to JsonSerializer

var json = JsonSerializer.Serialize(this.controller.GetAllProductsJson().Value);

Line 208: Swap AreEqual with That

Assert.That("[\"Table\",\"Chair\",\"Desk\",\"Computer\",\"Beer fridge\"]" == json);

I know this isn't related to what you're actually changing in this PR, but let's get this test fixed, then I'll create some issues for going through the rest of the tests, as they might also be using the legacy methods 💪

Hope it all makes sense 🤞

@erikjanwestendorp
Copy link
Contributor Author

@sofietoft Thanks for the review just updated the PR with your suggestions (including the unit test) 😄

@sofietoft
Copy link
Contributor

Looks great @erikjanwestendorp ! 👏
Thanks for applying the suggestions.

I'll make sure this is merged!

@sofietoft sofietoft merged commit 5907778 into umbraco:main Oct 11, 2024
1 check passed
@erikjanwestendorp erikjanwestendorp deleted the update-unit-testing branch October 11, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants